feat: update replicator retry logic #66

gk-kindred · 2023-08-09T02:34:24Z

No description provided.

Makefile

fmarek-kindred · 2023-08-09T03:00:26Z

examples/cohort_banking_with_sdk/examples/cohort_banking_with_sdk.rs

@@ -205,39 +198,6 @@ async fn main() -> Result<(), String> {
    Ok(())
 }

-fn start_queue_monitor(queue: Arc<Receiver<TransferRequest>>) -> JoinHandle<Result<(), String>> {


Why we are removing this?

It is not really needed. This was useful when replicator was part of cohort-sdk. I forgot to remove it in the earlier PR which was merged to master last week.

This one is observing how quickly cohort is draining the generated queue. It is not related to replicator. I prefer it to stay :) What you saw previously is that monitor was printing stats of generator queue and replicator queue (via heartbeat channel), but now it observes only generator. Btw if we remove it, I am not sure how system will stop itself, because async services are running forever, the don't know when to stop.

aaah okay, I must have got confused with the earlier logic. Will add this back.

The app stops correctly even after I remove it. In fact one reason why I removed it was, sometimes, the monitor would preemptively stop the app even before it reached the target number of txn. Probably there is a bug somewhere there, which I didn't dig too much in detail, but just deleted the code :)

It can happen when generated queue becomes empty but there is some work still queued on the replicator side. The monitor does not shut down immediately, but waits for some number of seconds. Of course if that wait period is not long enough then app can close too early. It is the matter of giving it more time to wait :)

codecov · 2023-08-09T03:03:19Z

Codecov Report

Patch coverage has no change and project coverage change: -0.16% ⚠️

Comparison is base (c23c658) 61.88% compared to head (47b8348) 61.73%.

❗ Current head 47b8348 differs from pull request most recent head b76dae0. Consider uploading reports for the commit b76dae0 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
- Coverage   61.88%   61.73%   -0.16%     
==========================================
  Files          88       89       +1     
  Lines        4909     4921      +12     
==========================================
  Hits         3038     3038              
- Misses       1871     1883      +12

Files Changed	Coverage Δ
...cohort_banking/src/callbacks/statemap_installer.rs	`0.00% <0.00%> (ø)`
packages/cohort_sdk/src/cohort.rs	`0.00% <0.00%> (ø)`
packages/talos_cohort_replicator/src/core.rs	`9.52% <ø> (+0.43%)`	⬆️
packages/talos_cohort_replicator/src/errors.rs	`0.00% <0.00%> (ø)`
packages/talos_cohort_replicator/src/lib.rs	`100.00% <ø> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fmarek-kindred · 2023-08-09T03:07:51Z

packages/cohort_banking/src/callbacks/statemap_installer.rs

+        loop {
+            let tx = cnn.transaction().await.map_err(|e| e.to_string())?;
+            if !statemap.is_empty() {
+                let updated_rows_res = BankStatemapInstaller::update_bank_transfer_request(&tx, &statemap, snapshot_version).await;


I suggest to rename this method "update_bank_transfer_request". By reading it feels like we are updating the request. However we are installing the given statemap.

will update this.

fmarek-kindred · 2023-08-09T03:13:53Z

packages/talos_cohort_replicator/src/services/statemap_installer_service.rs

@@ -11,13 +14,72 @@ pub struct StatemapInstallerConfig {
    pub thread_pool: Option<u16>,
 }

+async fn statemap_install_future(


RE: method name with ending _future. What does it mean? When reading it is not straightforward to understand what will happen when method is called.

Isn't future is pretty self explanatory in this context.

To me it was pretty obvious based on being used in the tokio task, that this fn is the future expected in the tokio task.

To me method name is about "do something" or "get something", "parse something", "create something" and etc From that context I don't know what _future does here. Why do we need to encode a data type into the method name :)

fmarek-kindred · 2023-08-09T05:37:01Z

packages/cohort_banking/src/callbacks/statemap_installer.rs

+const SNAPSHOT_UPDATE_QUERY: &str = r#"UPDATE cohort_snapshot SET "version" = ($1)::BIGINT WHERE id = $2 AND "version" < ($1)::BIGINT"#;
+
+fn is_retriable_error(error: &str) -> bool {
+    // TODO: improve retriable error detection when error moves from String to enum or struct.


Agree, this is Postgres text. Somehow it should be abstracted depending on DB type.

fmarek-kindred · 2023-08-09T05:37:12Z

packages/cohort_banking/src/callbacks/statemap_installer.rs

-            let mut cnn = self.database.get().await.map_err(|e| e.to_string())?;
-            let tx = cnn.transaction().await.map_err(|e| e.to_string())?;
+impl BankStatemapInstaller {
+    async fn install_bank_transfer_statemap(tx: &Transaction<'_>, statemap: &[StatemapItem], _snapshot_version: u64) -> Result<u64, String> {


packages/cohort_banking/src/callbacks/statemap_installer.rs

fmarek-kindred · 2023-08-09T05:40:00Z

packages/talos_cohort_replicator/src/services/statemap_installer_service.rs

+            );
+            replicator_tx
+                .send(ReplicatorChannel::InstallationFailure(format!(
+                    "Installed failed for version={version:?} error={err:?}"


Is it a typo in "Installed"?

yup will update this

fmarek-kindred

Looks good. Left minor typo warnings.

gk-kindred requested a review from fmarek-kindred August 9, 2023 02:34

gk-kindred added 8 commits August 9, 2023 12:36

feat: initial commit for replicator as standalone package

59980e6

feat: first working cut of replicator as a package with example

b1c3075

chore: remove old cohort package

2aaee04

feat: retry applied to banking statemap installer

3878c4f

feat: use env variables in config

16c7d2e

feat: remove replicator dependency from cohort-sdk

2b409e3

feat: initial commit for error handling

2862e6d

feat: update retry logic of replicator + installer

09249ba

fmarek-kindred reviewed Aug 9, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

gk-kindred force-pushed the feature/update-replicator-retry-logic branch from 7c396be to 09249ba Compare August 9, 2023 02:58

fmarek-kindred reviewed Aug 9, 2023

View reviewed changes

gk-kindred added 2 commits August 9, 2023 14:26

feat: remove give up logic and retry infinitly for retriable errors

fefdbda

chore: update inputs from review comments

2b25481

gk-kindred marked this pull request as ready for review August 9, 2023 04:51

fmarek-kindred reviewed Aug 9, 2023

View reviewed changes

packages/cohort_banking/src/callbacks/statemap_installer.rs Show resolved Hide resolved

fmarek-kindred reviewed Aug 9, 2023

View reviewed changes

fmarek-kindred approved these changes Aug 9, 2023

View reviewed changes

chore: updates from review comments

b76dae0

gk-kindred merged commit e680d4a into master Aug 9, 2023
2 checks passed

gk-kindred deleted the feature/update-replicator-retry-logic branch August 9, 2023 06:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update replicator retry logic #66

feat: update replicator retry logic #66

gk-kindred commented Aug 9, 2023

fmarek-kindred Aug 9, 2023

gk-kindred Aug 9, 2023

fmarek-kindred Aug 9, 2023

gk-kindred Aug 9, 2023

fmarek-kindred Aug 9, 2023 •

edited

Loading

codecov bot commented Aug 9, 2023 •

edited

Loading

fmarek-kindred Aug 9, 2023 •

edited

Loading

gk-kindred Aug 9, 2023

fmarek-kindred Aug 9, 2023

gk-kindred Aug 9, 2023

fmarek-kindred Aug 9, 2023

fmarek-kindred Aug 9, 2023

fmarek-kindred Aug 9, 2023

fmarek-kindred Aug 9, 2023

gk-kindred Aug 9, 2023

fmarek-kindred left a comment

feat: update replicator retry logic #66

feat: update replicator retry logic #66

Conversation

gk-kindred commented Aug 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmarek-kindred Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 9, 2023 • edited Loading

Codecov Report

fmarek-kindred Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmarek-kindred left a comment

Choose a reason for hiding this comment

fmarek-kindred Aug 9, 2023 •

edited

Loading

codecov bot commented Aug 9, 2023 •

edited

Loading

fmarek-kindred Aug 9, 2023 •

edited

Loading